Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix warning and offset voltage corruption after fw update or downgrade (#466 fix) #458

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

Combinacijus
Copy link
Contributor

Fixes #446 settings corruption bug.
I tested upgrade and downgrade both on emulator and goggles successfully
Now after firmware upgrade Warning Cell Voltage and Voltage Calibration will be reinitialized to default values and saved as calibration_offset_mv and voltage_mv in setting.ini.

From now on setting.ini will contain both new and old settings:
image

Should be good to merge

@Master92
Copy link
Contributor

Couldn't you implement some sort of migration so that when I upgrade, I don't have to re-set my limits?

if (ini_haskey("power", "calibration_offset", SETTING_INI)) {
    const long oldSetting = ini_getl("power", "calibration_offset", g_setting_defaults.power.calibration_offset, SETTING_INI);
    ini_puts("power", "calibration_offset", NULL, SETTING_INI); // Deletes the key/value from the file as per documentation 
    ini_puts("power", "calibration_offset_mv", oldSetting * 100, SETTING_INI); // I'm unsure about the factor, adjust as needed
}

and the same for voltage.

@Combinacijus
Copy link
Contributor Author

@Master92 I could but I I'm not sure if it's ok to write migration code just for a single firmware upgrade which will be kept for all subsequent versions because you can't know from which old version user will upgrade
Also it will use default values when downgrading and same default when upgrading again if no extra code is added to check for already existing *_mv setting

But I also see that such migration code will save all the users hassle of setup

@ligenxxxx what's your thoughts on this? Is such migration code ok? If so I can make a new commit

@ligenxxxx
Copy link
Member

It is best not to do any migration, the simpler the better. We just need to make sure that both upgrades and downgrades are OK.

@Master92
Copy link
Contributor

It is best not to do any migration, the simpler the better. We just need to make sure that both upgrades and downgrades are OK.

Okay I respect your decision. But please add a remark to the next release that users have to re-configure that setting after an upgrade.

@ligenxxxx
Copy link
Member

Okay I respect your decision. But please add a remark to the next release that users have to re-configure that setting after an upgrade.

Understand, this setting becomes the default value after the upgrade.

@ligenxxxx ligenxxxx merged commit e0fe2b1 into hd-zero:main Nov 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants